Add typing to membership Replication class methods #8809

anoadragon453 · 2020-11-24T22:03:17Z

This PR grew out of a discussion in #6739, and tries to do two things:

Add typing to method arguments
~~Remove unnecessary arguments on those methods~~

~~Removing unneeded arguments is fine as _serialize_payload is called as:~~

synapse/synapse/replication/http/_base.py

Line 169 in 1781bbe

data = await cls._serialize_payload(**kwargs)

~~and _handle_request called as:~~

synapse/synapse/replication/http/_base.py

Line 263 in 1781bbe

    
           return self.response_cache.wrap(txn_id, self._handle_request, request, **kwargs)

~~aka with the arguments request, **kwargs.~~ This doesn't work :)

You'll notice that there are a lot of # type: ignores in here. This is due to the base methods not matching the overloads here. This is necessary to stop mypy complaining, but if there's a better way than just blanket ignoring things I'd love to know!

I imagine there's likely further work needed in other replication classes, however seeing as the discussion above only concerned membership replication classes, that is all this particular PR addresses.

…thods

erikjohnston · 2020-11-25T11:08:53Z

Removing unneeded arguments is fine as _serialize_payload is called as:

I think that's backwards, we're passing in all the arguments but _serialize_payload doesn't have a **kwargs so will fail with "called with too many arguments".

(The link to the discussion doesn't work, I think because that PR is huge).

I'm not sure that I'd actually want to remove the "unnecessary" args from _serialize_payload as a) I think it makes sense for the serializiation function to have access to all vars passed in to the invocation, and b) at least I use it to actually see what the args for the client are (without having to cross reference to other places).

You'll notice that there are a lot of # type: ignores in here. This is due to the base methods not matching the overloads here. This is necessary to stop mypy complaining, but if there's a better way than just blanket ignoring things I'd love to know!

I think the "correct" way of doing this is to write a mypy plugin, like we have for cache functions. This would allow us to correctly set the signature of send_request to be the same as _serialize_payload so we actually get proper type checking.

(Removing review request as this breaks the http replication APIs)

anoadragon453 · 2020-11-25T12:00:49Z

@erikjohnston Thanks for the detailed explanation, sorry that this PR is a bit of a mess :)

Yep, these changes don't work, and thanks for the justification for keeping the arguments in. I'll rework this PR to just keep the added types, but bring back the removed args.

(The link to the discussion doesn't work, I think because that PR is huge).

Hm, it seems linking to a discussion via the /files endpoint doesn't work - but without that it resolves. Have updated the link.

I think the "correct" way of doing this is to write a mypy plugin, like we have for cache functions. This would allow us to correctly set the signature of send_request to be the same as _serialize_payload so we actually get proper type checking.

Oh, right! I'll file a followup issue for this then.

That file is really well documented btw.

erikjohnston · 2020-11-25T13:42:22Z

That file is really well documented btw.

Mainly because none of the functions are documented anywhere but in the source code and aaaaaaaaaargh

anoadragon453 · 2020-11-26T16:00:28Z

@erikjohnston Have brought the args back! I'm currently writing up an issue for the mypy plugin, however:

This would allow us to correctly set the signature of send_request to be the same as _serialize_payload so we actually get proper type checking.

Isn't the problem that the _serialize_payload and _handle_request functions don't match that of the superclass, ReplicationEndpoint? Or am I missing something here?

erikjohnston · 2020-11-26T16:24:54Z

This would allow us to correctly set the signature of send_request to be the same as _serialize_payload so we actually get proper type checking.

Isn't the problem that the _serialize_payload and _handle_request functions don't match that of the superclass, ReplicationEndpoint? Or am I missing something here?

That's a problem, yes. The other is that we don't have type hints on the function returned by make_client, this means that even if you add type hints to _serialize_payload mypy won't check that we're passing the correct args when we call it.

… functions" This reverts commit 3fb055c. It was found that these arguments are necessary and nice to keep around following a discussion in matrix-org#8809.

anoadragon453 · 2020-11-26T17:18:03Z

@erikjohnston Thanks for the explanation! I've created an issue here which hopefully covers it: #8828

@angdraug

Synapse 1.24.0 (2020-12-09) =========================== Due to the two security issues highlighted below, server administrators are encouraged to update Synapse. We are not aware of these vulnerabilities being exploited in the wild. Security advisory ----------------- The following issues are fixed in v1.23.1 and v1.24.0. - There is a denial of service attack ([CVE-2020-26257](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-26257)) against the federation APIs in which future events will not be correctly sent to other servers over federation. This affects all servers that participate in open federation. (Fixed in [#8776](matrix-org/synapse#8776)). - Synapse may be affected by OpenSSL [CVE-2020-1971](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1971). Synapse administrators should ensure that they have the latest versions of the cryptography Python package installed. To upgrade Synapse along with the cryptography package: * Administrators using the [`matrix.org` Docker image](https://hub.docker.com/r/matrixdotorg/synapse/) or the [Debian/Ubuntu packages from `matrix.org`](https://github.com/matrix-org/synapse/blob/master/INSTALL.md#matrixorg-packages) should ensure that they have version 1.24.0 or 1.23.1 installed: these images include the updated packages. * Administrators who have [installed Synapse from source](https://github.com/matrix-org/synapse/blob/master/INSTALL.md#installing-from-source) should upgrade the cryptography package within their virtualenv by running: ```sh <path_to_virtualenv>/bin/pip install 'cryptography>=3.3' ``` * Administrators who have installed Synapse from distribution packages should consult the information from their distributions. Internal Changes ---------------- - Add a maximum version for pysaml2 on Python 3.5. ([\#8898](matrix-org/synapse#8898)) Synapse 1.24.0rc2 (2020-12-04) ============================== Bugfixes -------- - Fix a regression in v1.24.0rc1 which failed to allow SAML mapping providers which were unable to redirect users to an additional page. ([\#8878](matrix-org/synapse#8878)) Internal Changes ---------------- - Add support for the `prometheus_client` newer than 0.9.0. Contributed by Jordan Bancino. ([\#8875](matrix-org/synapse#8875)) Synapse 1.24.0rc1 (2020-12-02) ============================== Features -------- - Add admin API for logging in as a user. ([\#8617](matrix-org/synapse#8617)) - Allow specification of the SAML IdP if the metadata returns multiple IdPs. ([\#8630](matrix-org/synapse#8630)) - Add support for re-trying generation of a localpart for OpenID Connect mapping providers. ([\#8801](matrix-org/synapse#8801), [\#8855](matrix-org/synapse#8855)) - Allow the `Date` header through CORS. Contributed by Nicolas Chamo. ([\#8804](matrix-org/synapse#8804)) - Add a config option, `push.group_by_unread_count`, which controls whether unread message counts in push notifications are defined as "the number of rooms with unread messages" or "total unread messages". ([\#8820](matrix-org/synapse#8820)) - Add `force_purge` option to delete-room admin api. ([\#8843](matrix-org/synapse#8843)) Bugfixes -------- - Fix a bug where appservices may be sent an excessive amount of read receipts and presence. Broke in v1.22.0. ([\#8744](matrix-org/synapse#8744)) - Fix a bug in some federation APIs which could lead to unexpected behaviour if different parameters were set in the URI and the request body. ([\#8776](matrix-org/synapse#8776)) - Fix a bug where synctl could spawn duplicate copies of a worker. Contributed by Waylon Cude. ([\#8798](matrix-org/synapse#8798)) - Allow per-room profiles to be used for the server notice user. ([\#8799](matrix-org/synapse#8799)) - Fix a bug where logging could break after a call to SIGHUP. ([\#8817](matrix-org/synapse#8817)) - Fix `register_new_matrix_user` failing with "Bad Request" when trailing slash is included in server URL. Contributed by @angdraug. ([\#8823](matrix-org/synapse#8823)) - Fix a minor long-standing bug in login, where we would offer the `password` login type if a custom auth provider supported it, even if password login was disabled. ([\#8835](matrix-org/synapse#8835)) - Fix a long-standing bug which caused Synapse to require unspecified parameters during user-interactive authentication. ([\#8848](matrix-org/synapse#8848)) - Fix a bug introduced in v1.20.0 where the user-agent and IP address reported during user registration for CAS, OpenID Connect, and SAML were of the wrong form. ([\#8784](matrix-org/synapse#8784)) Improved Documentation ---------------------- - Clarify the usecase for a msisdn delegate. Contributed by Adrian Wannenmacher. ([\#8734](matrix-org/synapse#8734)) - Remove extraneous comma from JSON example in User Admin API docs. ([\#8771](matrix-org/synapse#8771)) - Update `turn-howto.md` with troubleshooting notes. ([\#8779](matrix-org/synapse#8779)) - Fix the example on how to set the `Content-Type` header in nginx for the Client Well-Known URI. ([\#8793](matrix-org/synapse#8793)) - Improve the documentation for the admin API to list all media in a room with respect to encrypted events. ([\#8795](matrix-org/synapse#8795)) - Update the formatting of the `push` section of the homeserver config file to better align with the [code style guidelines](https://github.com/matrix-org/synapse/blob/develop/docs/code_style.md#configuration-file-format). ([\#8818](matrix-org/synapse#8818)) - Improve documentation how to configure prometheus for workers. ([\#8822](matrix-org/synapse#8822)) - Update example prometheus console. ([\#8824](matrix-org/synapse#8824)) Deprecations and Removals ------------------------- - Remove old `/_matrix/client/*/admin` endpoints which were deprecated since Synapse 1.20.0. ([\#8785](matrix-org/synapse#8785)) - Disable pretty printing JSON responses for curl. Users who want pretty-printed output should use [jq](https://stedolan.github.io/jq/) in combination with curl. Contributed by @tulir. ([\#8833](matrix-org/synapse#8833)) Internal Changes ---------------- - Simplify the way the `HomeServer` object caches its internal attributes. ([\#8565](matrix-org/synapse#8565), [\#8851](matrix-org/synapse#8851)) - Add an example and documentation for clock skew to the SAML2 sample configuration to allow for clock/time difference between the homserver and IdP. Contributed by @localguru. ([\#8731](matrix-org/synapse#8731)) - Generalise `RoomMemberHandler._locally_reject_invite` to apply to more flows than just invite. ([\#8751](matrix-org/synapse#8751)) - Generalise `RoomStore.maybe_store_room_on_invite` to handle other, non-invite membership events. ([\#8754](matrix-org/synapse#8754)) - Refactor test utilities for injecting HTTP requests. ([\#8757](matrix-org/synapse#8757), [\#8758](matrix-org/synapse#8758), [\#8759](matrix-org/synapse#8759), [\#8760](matrix-org/synapse#8760), [\#8761](matrix-org/synapse#8761), [\#8777](matrix-org/synapse#8777)) - Consolidate logic between the OpenID Connect and SAML code. ([\#8765](matrix-org/synapse#8765)) - Use `TYPE_CHECKING` instead of magic `MYPY` variable. ([\#8770](matrix-org/synapse#8770)) - Add a commandline script to sign arbitrary json objects. ([\#8772](matrix-org/synapse#8772)) - Minor log line improvements for the SSO mapping code used to generate Matrix IDs from SSO IDs. ([\#8773](matrix-org/synapse#8773)) - Add additional error checking for OpenID Connect and SAML mapping providers. ([\#8774](matrix-org/synapse#8774), [\#8800](matrix-org/synapse#8800)) - Add type hints to HTTP abstractions. ([\#8806](matrix-org/synapse#8806), [\#8812](matrix-org/synapse#8812)) - Remove unnecessary function arguments and add typing to several membership replication classes. ([\#8809](matrix-org/synapse#8809)) - Optimise the lookup for an invite from another homeserver when trying to reject it. ([\#8815](matrix-org/synapse#8815)) - Add tests for `password_auth_provider`s. ([\#8819](matrix-org/synapse#8819)) - Drop redundant database index on `event_json`. ([\#8845](matrix-org/synapse#8845)) - Simplify `uk.half-shot.msc2778.login.application_service` login handler. ([\#8847](matrix-org/synapse#8847)) - Refactor `password_auth_provider` support code. ([\#8849](matrix-org/synapse#8849)) - Add missing `ordering` to background database updates. ([\#8850](matrix-org/synapse#8850)) - Allow for specifying a room version when creating a room in unit tests via `RestHelper.create_room_as`. ([\#8854](matrix-org/synapse#8854))

Add typing and remove unnecessary arguments from Replication class me…

90683a5

…thods

anoadragon453 changed the title ~~Add typing and remove unnecessary arguments from Replication class methods~~ Add typing and remove unnecessary arguments from membership Replication class methods Nov 24, 2020

Changelog

175294e

anoadragon453 requested a review from a team November 24, 2020 22:05

anoadragon453 mentioned this pull request Nov 24, 2020

Implement knock feature #6739

Merged

7 tasks

erikjohnston removed the request for review from a team November 25, 2020 11:09

anoadragon453 added 2 commits November 26, 2020 15:45

Add arguments to _serialize_payload back. Add missing return types

ad13f9f

Drop some commas

377673d

anoadragon453 changed the title ~~Add typing and remove unnecessary arguments from membership Replication class methods~~ Add typing to membership Replication class methods Nov 26, 2020

anoadragon453 mentioned this pull request Nov 26, 2020

Create a mypy plugin for replication class methods #8828

Open

anoadragon453 requested a review from a team November 26, 2020 17:18

erikjohnston approved these changes Nov 27, 2020

View reviewed changes

anoadragon453 merged commit 5cbe8d9 into develop Nov 27, 2020

anoadragon453 deleted the anoa/replication_class_method_args branch November 27, 2020 10:49

matrixbot mentioned this pull request Dec 21, 2023

Create a mypy plugin for replication class methods element-hq/synapse#8828

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typing to membership Replication class methods #8809

Add typing to membership Replication class methods #8809

anoadragon453 commented Nov 24, 2020 •

edited

Loading

erikjohnston commented Nov 25, 2020 •

edited

Loading

anoadragon453 commented Nov 25, 2020

erikjohnston commented Nov 25, 2020

anoadragon453 commented Nov 26, 2020 •

edited

Loading

erikjohnston commented Nov 26, 2020

anoadragon453 commented Nov 26, 2020

Add typing to membership Replication class methods #8809

Add typing to membership Replication class methods #8809

Conversation

anoadragon453 commented Nov 24, 2020 • edited Loading

erikjohnston commented Nov 25, 2020 • edited Loading

anoadragon453 commented Nov 25, 2020

erikjohnston commented Nov 25, 2020

anoadragon453 commented Nov 26, 2020 • edited Loading

erikjohnston commented Nov 26, 2020

anoadragon453 commented Nov 26, 2020

anoadragon453 commented Nov 24, 2020 •

edited

Loading

erikjohnston commented Nov 25, 2020 •

edited

Loading

anoadragon453 commented Nov 26, 2020 •

edited

Loading